feat: SecretManagerSecret full test coverage & manual replication field #3371

yuwenma · 2024-12-12T05:37:28Z

Improve SecretManager with full test coverage.
2nd git-commit is real gcp
3rd git-commit is mockgcp

Filed #3395 to capture the TTL issue

yuwenma · 2024-12-12T05:43:20Z

apis/secretmanager/v1beta1/secret_types.go

@@ -48,8 +48,7 @@ type SecretManagerSecretSpec struct {
 	//  This is always provided on output, regardless of what was sent on input.
 	ExpireTime *string `json:"expireTime,omitempty"`

-	// Input only. The TTL for the
-	//  [Secret][google.cloud.secretmanager.v1.Secret].
+	// Input only. A duration in seconds with up to nine fractional digits, ending with 's'. Example: "3.5s".


The TTL is a stale field that causes customers issues: it uses "Now()" when the resource is created that users cannot control or predict.
But since this is already in Beta, we still add the test coverage for it.

Also this change improves the user experience a little bit otherwise they don't now what value to give.

Ouch. This doesn't feel like a spec field, because if I specify ttl: 60s what is the TTL value in 10 seconds? It seems like it should be 60s, because that is my desired state. But I suspect it will be 50s.

Yes, this is exactly the problem!

yuwenma · 2024-12-12T05:45:43Z

dev/tasks/run-e2e

@@ -26,7 +26,7 @@ if [[ -z "${KUBEBUILDER_ASSETS:-}" ]]; then
 fi

 if [[ -z "${KCC_USE_DIRECT_RECONCILERS:-}" ]]; then
-  KCC_USE_DIRECT_RECONCILERS=ComputeForwardingRule,GKEHubFeatureMembership,SecretManagerSecret,SecretManagerSecretVersion


The test now covers both legacy controller and direct controller (via alpha direct annotation). we no longer need this

yuwenma · 2024-12-12T05:47:13Z

pkg/controller/direct/secretmanager/secret_controller.go

+		if secret.Spec.Replication.UserManaged != nil {
+			for _, r := range secret.Spec.Replication.UserManaged.Replicas {
+				if r.CustomerManagedEncryption != nil {
+					kmsKeyRef := r.CustomerManagedEncryption.KmsKeyRef


I previously missed this field because I thought it is another field spec.customerManagedEncryption, which is not supported yet in the current released beta API.

Do we have a fuzz test :-)

PR on the way.

.../resourcefixture/testdata/basic/secretmanager/v1beta1/secretmanagersecretversion/create.yaml

jasonvigil · 2024-12-13T21:01:42Z

/lgtm

justinsb · 2024-12-16T14:22:15Z

mockgcp/mocksecretmanager/secrets.go

@@ -84,6 +84,13 @@ func (s *SecretsV1) populateDefaultsForSecret(ctx context.Context, obj *pb.Secre
 			return fmt.Errorf("Aliases cannot be assigned to versions that don't exist")
 		}
 	}
+	// TTL and ExpireTime are OneOf, but the GCP service always converts TTL to expireTime before storing the object.


This is a reasonable behaviour. Maybe we should deprecate spec.ttl (and replace with spec.initialTTL - although if we are going to do that, we can just change the comment on the field)

justinsb · 2024-12-16T14:23:33Z

mockgcp/mocksecretmanager/secrets.go

@@ -154,6 +161,10 @@ func (s *SecretsV1) UpdateSecret(ctx context.Context, req *pb.UpdateSecretReques
 			updated.Expiration = &pb.Secret_ExpireTime{
 				ExpireTime: req.Secret.GetExpireTime(),
 			}
+		case "ttl":
+			updated.Expiration = &pb.Secret_Ttl{
+				Ttl: req.Secret.GetTtl(),


Nit: really? I would have assumed it would set Expiration.ExpireTime, just like in create. Still if this is what the logs tell us....

Actually I don't know if we have test coverage of this in this PR (?). It's not a big deal, because it's a mock (we can fix it when we need it), just that I would have guessed the other way!

In general I think the behaviour of this ttl field is very tricky. Should we reconcile it (meaning secrets never expire)? Should we only apply on create? But if we do that, what happens when the user changes the TTL field in KRM? What happens if they change a different field?

Yes... here's what the SecertManager server tells us https://screenshot.googleplex.com/9giJXnHPt3nMSwm.png For the log, the respond is always expire time, no ttl whatever the request says. I uploaded the real log in the git-commit

Oh I see. We set ttl, and then in populateDefaultsForSecret we convert it. I guess that works, because then we can use the shared fieldmask update code at some point in the future!

justinsb · 2024-12-16T20:46:41Z

mockgcp/mocksecretmanager/secrets.go

@@ -84,6 +84,13 @@ func (s *SecretsV1) populateDefaultsForSecret(ctx context.Context, obj *pb.Secre
 			return fmt.Errorf("Aliases cannot be assigned to versions that don't exist")
 		}
 	}
+	// TTL and ExpireTime are OneOf, but the GCP service always converts TTL to expireTime before storing the object.
+	if obj.GetTtl() != nil {
+		expirateTime := timestamppb.Now().AsTime().Add(obj.GetTtl().AsDuration())


Nit: might as well name it the same as the field you're setting, i.e. expireTime

justinsb · 2024-12-16T20:47:19Z

pkg/controller/direct/secretmanager/secret_controller.go

-
+	if paths.Has("ttl") {
+		paths = paths.Delete("ttl")
+		resource.Expiration = a.actual.Expiration


Nit: there should be a comment here, because I don't think this does anything unless expiration is set in paths

google-oss-prow · 2024-12-16T20:47:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [justinsb]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

yuwenma assigned justinsb and jingyih Dec 12, 2024

yuwenma changed the title ~~feat: add SecretManager manual replication feature~~ feat: SecretManagerSecret full test coverage & manual replication field Dec 12, 2024

yuwenma commented Dec 12, 2024

View reviewed changes

yuwenma force-pushed the secret-manager-wip branch 3 times, most recently from 08c7091 to 5aa4c54 Compare December 12, 2024 06:36

feat: add SecretManager manual replication feature

94a7548

yuwenma force-pushed the secret-manager-wip branch 4 times, most recently from 16438ad to c827a86 Compare December 12, 2024 07:34

real gcp log

afde6b9

yuwenma force-pushed the secret-manager-wip branch from c827a86 to 4e61488 Compare December 12, 2024 07:39

mock gcp log

32a002c

yuwenma force-pushed the secret-manager-wip branch from 4e61488 to 32a002c Compare December 12, 2024 07:40

add alpha-direct annotation to secretversion test suite

f2edfe4

jasonvigil reviewed Dec 13, 2024

View reviewed changes

.../resourcefixture/testdata/basic/secretmanager/v1beta1/secretmanagersecretversion/create.yaml Show resolved Hide resolved

record vcr

52f0b95

yuwenma force-pushed the secret-manager-wip branch from d810d99 to 52f0b95 Compare December 13, 2024 19:58

google-oss-prow bot assigned jasonvigil Dec 13, 2024

google-oss-prow bot added the lgtm label Dec 13, 2024

justinsb reviewed Dec 16, 2024

View reviewed changes

yuwenma added 2 commits December 16, 2024 17:50

option 1 with real GCP

8825382

mock gcp

792894d

google-oss-prow bot removed the lgtm label Dec 16, 2024

justinsb reviewed Dec 16, 2024

View reviewed changes

justinsb approved these changes Dec 16, 2024

View reviewed changes

google-oss-prow bot added the lgtm label Dec 16, 2024

google-oss-prow bot added the approved label Dec 16, 2024

google-oss-prow bot merged commit ff59285 into GoogleCloudPlatform:master Dec 16, 2024
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: SecretManagerSecret full test coverage & manual replication field #3371

feat: SecretManagerSecret full test coverage & manual replication field #3371

yuwenma commented Dec 12, 2024 •

edited

Loading

yuwenma Dec 12, 2024 •

edited

Loading

justinsb Dec 16, 2024

yuwenma Dec 16, 2024

yuwenma Dec 12, 2024

yuwenma Dec 12, 2024

justinsb Dec 16, 2024

yuwenma Dec 16, 2024

jasonvigil commented Dec 13, 2024

justinsb Dec 16, 2024

justinsb Dec 16, 2024

justinsb Dec 16, 2024

yuwenma Dec 16, 2024 •

edited

Loading

justinsb Dec 16, 2024

justinsb Dec 16, 2024

justinsb Dec 16, 2024

google-oss-prow bot commented Dec 16, 2024

feat: SecretManagerSecret full test coverage & manual replication field #3371

feat: SecretManagerSecret full test coverage & manual replication field #3371

Conversation

yuwenma commented Dec 12, 2024 • edited Loading

yuwenma Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonvigil commented Dec 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuwenma Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

google-oss-prow bot commented Dec 16, 2024

yuwenma commented Dec 12, 2024 •

edited

Loading

yuwenma Dec 12, 2024 •

edited

Loading

yuwenma Dec 16, 2024 •

edited

Loading